Structured Logs: Add SentrySwiftLog Integration
#6286
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6286 +/- ##
========================================
Coverage ? 87.012%
========================================
Files ? 453
Lines ? 37745
Branches ? 17484
========================================
Hits ? 32843
Misses ? 4856
Partials ? 46
Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bc4bb46 | 1235.63 ms | 1264.24 ms | 28.61 ms |
| cd9727b | 1236.04 ms | 1254.41 ms | 18.37 ms |
| 0759f32 | 1237.10 ms | 1257.35 ms | 20.25 ms |
| 7aaa0b6 | 1230.49 ms | 1263.78 ms | 33.29 ms |
| c4b0360 | 1235.68 ms | 1252.65 ms | 16.97 ms |
| 83e9b16 | 1223.25 ms | 1250.94 ms | 27.69 ms |
| 18be519 | 1241.22 ms | 1260.57 ms | 19.35 ms |
| 8ea5293 | 1242.70 ms | 1262.25 ms | 19.55 ms |
| 1a820a1 | 1217.79 ms | 1260.34 ms | 42.55 ms |
| 35c962f | 1207.61 ms | 1235.90 ms | 28.29 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| bc4bb46 | 23.75 KiB | 980.81 KiB | 957.07 KiB |
| cd9727b | 23.75 KiB | 879.25 KiB | 855.51 KiB |
| 0759f32 | 23.75 KiB | 880.20 KiB | 856.46 KiB |
| 7aaa0b6 | 23.75 KiB | 989.97 KiB | 966.23 KiB |
| c4b0360 | 23.75 KiB | 946.69 KiB | 922.94 KiB |
| 83e9b16 | 23.75 KiB | 947.72 KiB | 923.97 KiB |
| 18be519 | 23.75 KiB | 926.64 KiB | 902.90 KiB |
| 8ea5293 | 23.75 KiB | 852.24 KiB | 828.49 KiB |
| 1a820a1 | 23.75 KiB | 994.82 KiB | 971.07 KiB |
| 35c962f | 23.75 KiB | 854.77 KiB | 831.02 KiB |
Previous results on branch: denrase/swift-log-sentry-poc
Startup times
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0fd08ab | 1225.14 ms | 1256.49 ms | 31.35 ms |
| b52b178 | 1227.86 ms | 1255.65 ms | 27.79 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 0fd08ab | 23.75 KiB | 995.43 KiB | 971.68 KiB |
| b52b178 | 23.75 KiB | 989.97 KiB | 966.22 KiB |
# Conflicts: # Package.swift # Sentry.xcodeproj/project.pbxproj
denrase
changed the title
swift-log IntegrationSentrySwiftLog Integration
denrase
requested review from
noahsmartin,
philipphofmann and
philprime
as code owners
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| # Clean up orphaned commas and fix syntax | ||
| sed -i '' '/^[[:space:]]*,$/d' $PACKAGE_FILE | ||
| sed -i '' 's/name: "Sentry\(-.*\)\?"$/name: "Sentry\1",/g' $PACKAGE_FILE | ||
| sed -i '' 's/platforms: \[\.iOS(\.v11), \.macOS(\.v10_13), \.tvOS(\.v11), \.watchOS(\.v4)\]$/platforms: [.iOS(.v11), .macOS(.v10_13), .tvOS(.v11), .watchOS(.v4)],/g' $PACKAGE_FILE |
There was a problem hiding this comment.
What is this trying to do?
There was a problem hiding this comment.
One of the CI jobs is replacing dependencies with local build artefacts. This broke, as swift-log was added to our Package.swift. This code fixes those issues.
There was a problem hiding this comment.
Does this only happen because of swift-log or should we do the fix in a separate PR?
There was a problem hiding this comment.
Only because of swift-log dependency added in our Package.swift
| .library(name: "SentrySwiftUI", targets: ["Sentry", "SentrySwiftUI", "SentryCppHelper"]), | ||
| .library(name: "SentryDistribution", targets: ["SentryDistribution"]) | ||
| .library(name: "SentryDistribution", targets: ["SentryDistribution"]), | ||
| .library(name: "SentrySwiftLog", targets: ["Sentry", "SentrySwiftLog"]) |
There was a problem hiding this comment.
m: I don't fully understand why this is another target. Is this only here because swift-log is a new dependency? Shouldn't we add it as a dependency to Sentry instead?
There was a problem hiding this comment.
The intent was to only pull in the dependency for users that actually are using swift-log, but then again, I am not sure how this would behave if a user is not using SentrySwiftLog target. Will check.
There was a problem hiding this comment.
Ok, there is a proposal that was partially adopted:
I created a sample app with a similar setup. While SPM will always resolve the dependency, it will bot be linked/build if it is not required by the target.
So this setup works as expected.
| platforms: [.iOS(.v15), .macOS(.v12), .tvOS(.v15), .watchOS(.v8)], | ||
| products: products, | ||
| dependencies: [ | ||
| .package(url: "https://github.com/apple/swift-log", from: "1.6.0") |
There was a problem hiding this comment.
h: So I think this is going to be a problem as soon as new swift-log version becomes available. AFAIK the SPM doesn't support peer-dependencies, so if someone uses swift-log with a version not being 1.6.0, while we depend on it, Xcode will not build because it doesn't support the same dependency with different versions like NPM.
So basically this will lock our SDK users into exactly the swift-log version we declare, requiring us to release a new SDK version with every update of swift-log, furthermore requiring users to update the Sentry SDK even though they just want a new version of swift-log. At this point I do not have a proposal how to fix this other than asking the user to "inject" their swift-log version by making it conform to a Sentry-included protocol
There was a problem hiding this comment.
Yeah, this one is difficult. We are depending on 1.6.0 < 2.0.0, so future minor versions should be covered and correctly resolved if users set their version to 1.8.0 for example, no? Assuming swift-log is not breaking current API with minor version updates.
One way would be to move this to a separate repository. While the same issue remains, we'd not be forced to create a new release in this repo, but rather just update the swift-log-sentry repo. This comes with the whole overhead of maintaining another repo, so I'd say this may not be worth it.
Another possibility would be to see if we can support lower versions of swift-log, for example 1.3.0 up to (excluding) 2.0.0, but then again if there is no demand for older minor versions, there's not much use in supporting those.
As long as swift-log is not regularly changing their major version, the cases where we need to do a release because of breaking swift-log API should be rare. If minor versions introduce additional features which are non-breaking that are useful to us, we need to update our code anyway.
# Conflicts: # Sentry.xcodeproj/project.pbxproj
3 participants
📜 Description
The Sentry
swift-logintegration.Setup
SentrySwiftLogas a target toSentrytargetSentrySwiftLogtoPackage.swiftiOS15-SwiftUiapp so you can play around with itImplementation
SentryLogHandlersentry-logmetadata is mapped to attributes and prefixed withswift-logDiscussion
Here vs Own Repo
Added the integration to this repo, because it is easier for us to maintain.
On the
swift-logrepo integrations section is linking to the swift package index with search termswift-log. So for visibility, we'd need to have a separateswift-log-sentryrepo.https://github.com/apple/swift-log?tab=readme-ov-file#available-log-handler-backends
https://swiftpackageindex.com/search?query=swift-log
Distribution
Only distributed through SPM, as
swift-logdropped support for CocoaPods. Also, we'll not be able to distribute this as a binary, asswift-logis not supporting that.💡 Motivation and Context
Closes #5372
💚 How did you test it?
Unit Tests
Sample App
📝 Checklist
You have to check all boxes before merging:
sendDefaultPIIis enabled.